Add multithreaded variant to ICU port #15592
Conversation
tests/other/test_pthread_icu.cpp
Outdated
| ustr.toUTF8String(str); | ||
| std::cout << str << std::endl; | ||
| }).detach(); | ||
| } No newline at end of file |
tests/other/test_pthread_icu.cpp
Outdated
|
|
||
| // Test that code using both pthread and icu compiles. | ||
| int main () { | ||
| std::thread([] { |
There was a problem hiding this comment.
I know we are not yet consistent, but we are trying to use a consistent style these days which involves 2-space indentation (we have a .clang-format file if you want to just use that)
There was a problem hiding this comment.
Sure. Can I recommend mentioning the existence of a style convention in Contributing or Developer's Guide?
tests/test_other.py
Outdated
| self.do_other_test('test_pthread_out_err.c') | ||
|
|
||
| @node_pthreads | ||
| def test_icu_mt(self): |
There was a problem hiding this comment.
Can you name the test the same as the file, i.e def test_pthread_icu?
tests/test_other.py
Outdated
|
|
||
| @node_pthreads | ||
| def test_icu_mt(self): | ||
| self.do_smart_test(test_file('other/test_pthread_icu.cpp'), emcc_args=['-pthread', '-sUSE_ICU', '-sPTHREAD_POOL_SIZE_STRICT=4', '-sEXIT_RUNTIME']) |
There was a problem hiding this comment.
Looks like do_other_test (like the test above) should suffice.
tests/other/test_pthread_icu.cpp
Outdated
| icu::UnicodeString ustr("Hello world!"); | ||
| ustr.toUTF8String(str); | ||
| std::cout << str << std::endl; | ||
| }).detach(); |
There was a problem hiding this comment.
Better to .join here since emscripten doesn't handling the case where the main application thread exits while other threads are still running.
Probably best to use PROXY_TO_PTHREAD and drop PTHREAD_POOL_SIZE_STRICT
|
I wonder if we can perhaps just always build with pthread enabled since browser should accept atomic opcode in single-threaded programs these days. See WebAssembly/threads#144 @tlively WDTY? |
Certain browsers do not support atomics: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Atomics |
|
Yeah, I think Safari still does not support atomics yet (according to https://webassembly.org/roadmap/), so it's probably best not to enable threads by default :( |
3 participants
Fixes #14518